Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add dependency_names method to EasyConfig class to get set of names of (direct) dependencies #4360

Merged
merged 2 commits into from
Oct 27, 2023
Merged

add dependency_names method to EasyConfig class to get set of names of (direct) dependencies #4360

merged 2 commits into from
Oct 27, 2023

Conversation

Flamefire
Copy link
Contributor

This is useful for determining if a feature or configure flag should be enabled. Especially as the pattern (getting the name out of ec.dependencies()) is used often already.

I considered adding it to EasyBlock (at least additionally) which is possible but IMO doesn't provide much benefit given that it saves only 3 chars of typing ('.ec')

I changed some tests to add testing or using this function which should be enough.

One "issue" is the handling of external modules which have a "name" of None. I currently filter them out. We could try reading ['external_module_metadata'].get('name', [])
However that would be a change in behavior making a straight replacement of current usages risky and I'm also not sure how to correctly handle that: external_module_metadata['name'] is a list. Should all be added?

@Flamefire Flamefire mentioned this pull request Oct 25, 2023
Merged
@Flamefire
Explicitely use set() instead of {}
8c9b09a 
Some Python version may interpret the latter as an (empty) dict instead
of a set
boegel
boegel approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@boegel boegel added this to the next release (4.8.2?) milestone Oct 27, 2023
@easybuilders easybuilders deleted a comment from boegelbot Oct 27, 2023
@boegel
Copy link
Member

boegel commented Oct 27, 2023

W.r.t. external dependencies: if we want to also get the names of those from dependency_names, we should just make sure that name is always defined for them, we don't want to add special handling for external deps all throughout the code imho.

@boegel boegel changed the title add EasyConfig.dependency_names add dependency_names method to EasyConfig class to get set of names of (direct) dependencies Oct 27, 2023
@boegel boegel merged commit b3bcb19 into easybuilders:develop Oct 27, 2023
39 checks passed
@Flamefire
Copy link
Contributor Author

W.r.t. external dependencies: if we want to also get the names of those from dependency_names, we should just make sure that name is always defined for them, we don't want to add special handling for external deps all throughout the code imho.

That is not possible: name inside a dependency is a string while it is a list of strings inside the external module info. So it only makes sense to insert that list into the result of such a function but not into the name property which would suddenly become either a string or a list.

@Flamefire Flamefire deleted the dependency_names branch October 27, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boegel boegel boegel approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
4.8.2
Development

Successfully merging this pull request may close these issues.
2 participants
@Flamefire @boegel